Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: treat attachments as binary by default #1007

Merged
merged 1 commit into from
Nov 1, 2024
Merged

Conversation

nicsor
Copy link
Contributor

@nicsor nicsor commented Oct 4, 2024

This PR updates the handling of attachments on events.

Resolves: #706

Context:
When encountaring binary data, the client sets a number of placeholders in the first package and then sends the actual data in the packets following it without additional info prepended.

although they state here that
each Socket.IO packet is wrapped in a Engine.IO message packet, so they will be prefixed by the character "4" when sent over the wire.

it does not seem to apply to attachements, as seen in this example:

      socket.send(
        '452-["message",{"_placeholder":true,"num":0},{"_placeholder":true,"num":1}]'
      );
      socket.send(Uint8Array.from([1, 2, 3]));
      socket.send(Uint8Array.from([4, 5, 6]));

      const packets = await waitForPackets(socket, 3);

When encountaring binary data, the client sets a number of
placeholders in the first package and then sends the actual
data in the packets following it without additional info
prepended.

Signed-off-by: Nicolae Natea <[email protected]>
@mrniko
Copy link
Owner

mrniko commented Oct 31, 2024

Why did you delete parseBinary() method?

@nicsor
Copy link
Contributor Author

nicsor commented Oct 31, 2024

I simply refactored the code for clarity.

It just seems like a big change on the diff because of the indentation.
addAttachement is basically parseBinary without the first checks, which are the main issue, as mentioned in the description.

@mrniko mrniko added this to the 2.0.12 milestone Nov 1, 2024
@mrniko mrniko added the bug label Nov 1, 2024
@mrniko mrniko merged commit f4a793f into mrniko:master Nov 1, 2024
2 checks passed
@mrniko
Copy link
Owner

mrniko commented Nov 1, 2024

Thanks for contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PacketType.valueOf throws IllegalStateException
2 participants